Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for expanding dot env variables in scripts #4979

Merged
merged 7 commits into from
Mar 12, 2022

Conversation

matteius
Copy link
Member

@matteius matteius commented Mar 11, 2022

Ensure that dot env variables are actually in the real environment so they can be exapnded by os.path.expandvars

The issue

#4975
#3901

The fix

The problem is that in the current code we are loading the .env variables into a copy of the environment which is not available to os.path.expandvars so they are always treated as a string. This fixes that by applying it to the environment before copying it. Also refactor away some of the complexity of load_dot_env.

The checklist

  • Associated issue
  • A news fragment in the news/ directory to describe this fix with the extension .bugfix, .feature, .behavior, .doc. .vendor. or .trivial (this will appear in the release changelog). Use semantic line breaks and name the file after the issue number or the PR #.

… they can be exapnded by os.path.expandvars
@matteius matteius changed the title Fix for Fix for expanding dot env variables in scripts Mar 11, 2022
pipenv/core.py Show resolved Hide resolved
@@ -2473,8 +2471,8 @@ def do_run(project, command, args, three=None, python=False, pypi_mirror=None):
project, three=three, python=python, validate=False, pypi_mirror=pypi_mirror,
)

load_dot_env(project)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am afraid this will break the isolation in test cases. dotenv files in one test case will contaminate other cases env.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@frostming Could you elaborate on this? Right now the test suite is passing with this change so I am not sure I understand, but maybe I do. If its a testing concern, shouldn't it be on a top level test fixture to cleanup the environment before or after each test?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue without this change is that environment that gets used for running the script is the environment without the dotenv variables. Is there a better way to accomplish it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't it be on a top level test fixture to cleanup the environment before or after each test?

Oh yeah, it is cleaned up.

@oz123 oz123 merged commit 8987b59 into main Mar 12, 2022
@oz123 oz123 deleted the issue-4975-env-variables-not-expanded branch March 12, 2022 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants